Skip to content

Patch rack-accept to support all valid RFC6838 characters in media types#1179

Merged
dblock merged 1 commit into
ruby-grape:masterfrom
suan:really_support_rfc6838
Oct 9, 2015
Merged

Patch rack-accept to support all valid RFC6838 characters in media types#1179
dblock merged 1 commit into
ruby-grape:masterfrom
suan:really_support_rfc6838

Conversation

@suan

@suan suan commented Oct 7, 2015

Copy link
Copy Markdown
Contributor

Continuation and completion of #1170

This patches rack-accept in a manner similar to how virtus was patched (as discussed)

@suan suan force-pushed the really_support_rfc6838 branch from 14d12a4 to 270a28d Compare October 7, 2015 14:09
@dblock

dblock commented Oct 7, 2015

Copy link
Copy Markdown
Member

For reference this has been PRed into rack-accept as mjackson/rack-accept#17.

@suan

suan commented Oct 7, 2015

Copy link
Copy Markdown
Contributor Author

Getting Gem::InstallError: rack-cache requires Ruby version >= 2.0.0 errors on the Rails3 Gemfile jruby build in Travis... but I don't see how these changes would cause that 😕

@dblock

dblock commented Oct 7, 2015

Copy link
Copy Markdown
Member

Maybe there was a new release of rack-cache or something like that? Would you try to hunt it down please? I'll kick it manually again see if it's intermittent.

@dblock

dblock commented Oct 7, 2015

Copy link
Copy Markdown
Member

Yep, rack-cache was released yesterday, https://rubygems.org/gems/rack-cache. So something broke there, lets fix that first?

@dblock

dblock commented Oct 7, 2015

Copy link
Copy Markdown
Member

rtomayko/rack-cache#120

@suan

suan commented Oct 7, 2015

Copy link
Copy Markdown
Contributor Author

Yeah, this seems tricky. The way I'm reading it is that Rails itself on Ruby1.9 will break now and will need to be updated, as actionpack 3.x will try to install rack-cache v1.3: https://github.com/rails/rails/blob/v3.2.22/actionpack/actionpack.gemspec

@dblock

dblock commented Oct 7, 2015

Copy link
Copy Markdown
Member

I would be ok to force an older version of rack-cache in Grape's gemfile. Want to try?

@suan

suan commented Oct 7, 2015

Copy link
Copy Markdown
Contributor Author

Sure, on it

@suan suan force-pushed the really_support_rfc6838 branch from 270a28d to 7bb76ee Compare October 7, 2015 15:30
@rnubel

rnubel commented Oct 7, 2015

Copy link
Copy Markdown
Contributor

First of all, hi @suan! So from what I can tell, only Rails 3 requires rack-cache, so I'd suggest we put this into the Appraisals file for rails-3.

@suan suan force-pushed the really_support_rfc6838 branch from 7bb76ee to d9427e2 Compare October 7, 2015 16:01
@suan

suan commented Oct 7, 2015

Copy link
Copy Markdown
Contributor Author

Hey @rnubel !! 😄 So I tried putting it in Appraisals instead of the Gemfile but it doesn't seem to have the desired effect, whereas having it in the Gemfile itself worked... could u take a look and see whether my changes were what you had in mind?

@rnubel

rnubel commented Oct 7, 2015

Copy link
Copy Markdown
Contributor

@suan: you have to run appraisal install, to re-generate the gemfiles/* files. That could be part of the Travis build, maybe, but right now it's manual.

@suan suan force-pushed the really_support_rfc6838 branch from d9427e2 to 27f4a54 Compare October 8, 2015 17:19
@suan

suan commented Oct 8, 2015

Copy link
Copy Markdown
Contributor Author

Thanks @rnubel - done!

@dblock Everything outside of the "Allowed Failures" section has passed. The very last specs in that section has been running for almost an hour tho and I think it's just TravisCI wonkiness?

@suan suan force-pushed the really_support_rfc6838 branch from 27f4a54 to fe63400 Compare October 9, 2015 00:39
@suan

suan commented Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

Force-pushed again and Travis is 💚! :)

dblock added a commit that referenced this pull request Oct 9, 2015
Patch rack-accept to support all valid RFC6838 characters in media types
@dblock dblock merged commit 0924100 into ruby-grape:master Oct 9, 2015
@dblock

dblock commented Oct 9, 2015

Copy link
Copy Markdown
Member

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants